Replace DvInt* with .NET standard data types. WIP by codemzs · Pull Request #683 · dotnet/machinelearning

codemzs · 2018-08-16T05:50:57Z

This change also removes missing value handling for sbyte, short, int and long because default of these values is a null and that does not fit well with sparse vector architecture where default for missing values is a zero. fixes #673

…to types

TomFinley · 2018-08-16T12:52:54Z

+            RegisterSimpleCodec(new UnsafeTypeCodec<sbyte>(this));
            RegisterSimpleCodec(new UnsafeTypeCodec<byte>(this));
-            RegisterSimpleCodec(new UnsafeTypeCodec<DvInt2>(this));
+            RegisterSimpleCodec(new UnsafeTypeCodec<Int16>(this));


Int16 [](start = 52, length = 5)

short, int, long, not Int16, Int32, Int64. #Closed

TomFinley · 2018-08-16T12:54:43Z

            }

-            private void GetCategoricalSlotRanges(int iiinfo, ref VBuffer<DvInt4> dst)
+            private void GetCategoricalSlotRanges(int iiinfo, ref VBuffer<Int32> dst)


Int32 [](start = 74, length = 5)

Please just use the keywords for types (so, in this case int), here and everywhere. #Closed

I made this change across the code base using find-replace. #Resolved

TomFinley · 2018-08-16T12:56:52Z


-            public void Conv(ref long? src, ref DvInt8 dst) => dst = src ?? DvInt8.NA;
+            public void Conv(ref long? src, ref Int64? dst) => dst = src;



I see there are no conversions from float to float, or double to double. So: are these conversions actually necessary? #Resolved

I have updated this code to not assume there can be nullables but I do see a conversion between byte and byte. @mandyshieh can you please take a look? #Resolved

TomFinley · 2018-08-16T13:00:53Z

    public RefPredicate<T> GetIsDefaultPredicate<T>(ColumnType type)

You will need to be careful here. We want the "default" of the new nullable integer types to be 0, since that's both a more sensible choice and it retains sparsity preservation for conversion from int? to int. (For that reason it may be beneficial to choose a better name than "default" since it will no longer be identical with C# sparsity.)

The implications for things like VBufferUtils.Densify are interesting.

The sparsity question will be quite far reaching, and will be by far the most challenging part of this change. #Resolved

Refers to: src/Microsoft.ML.Data/Data/Conversion.cs:703 in a0ceabb. [](commit_id = a0ceabb, deletion_comment = False)

TomFinley · 2018-08-16T13:10:05Z

+            RegisterSimpleCodec(new UnsafeTypeCodec<sbyte>(this));
            RegisterSimpleCodec(new UnsafeTypeCodec<byte>(this));
-            RegisterSimpleCodec(new UnsafeTypeCodec<DvInt2>(this));
+            RegisterSimpleCodec(new UnsafeTypeCodec<Int16>(this));


RegisterSimpleCodec(new UnsafeTypeCodec(this)); [](start = 12, length = 54)

We will want the nullable types to be serializable. #Closed

This is not needed since there will no nullable types 👍 #Resolved

Good, thanks @codemzs. Incidentally were you going to update the description of #673 pursuant to our discussions yesterday? It is still the old description.

Edit: Actually I don't think you've linked to that issue from this PR at all. You should, maybe "fixes #673" in your description.

In reply to: 210744044 [](ancestors = 210744044)

…ort for ints.

…to types

codemzs · 2018-08-16T22:31:20Z

 namespace Microsoft.ML.Runtime.Internal.Utilities
 {
-    // Reasonable choices are Double and System.Int64.
+    // Reasonable choices are Double and System.long.


long [](start = 48, length = 4)

fix this #Resolved

eerhardt · 2018-08-17T16:20:03Z

-                        {
-                            Ch.Assert(colType.ItemType == NumberType.I4);
-                            return CreateConvertingArrayGetterDelegate<int?, DvInt4>(index, x => x ?? DvInt4.NA);
+                            return CreateConvertingArrayGetterDelegate<int, int>(index, x => x);


Why do we need a "converting" delegate here to return the same value? #Resolved

…to types

eerhardt · 2018-08-17T16:24:05Z

-            if (type == typeof(DvInt1) || type == typeof(sbyte) || type == typeof(sbyte?))
+            if (type == typeof(sbyte))
                kind = DataKind.I1;
            else if (type == typeof(byte) || type == typeof(byte?))


byte? should be removed here as well, to follow the rest of the changes. #Resolved

Why is "byte?" mapped to a U1? I thought U1 just mapped to a byte and did not support nullables ....this needs to go away anyways

In reply to: 210963225 [](ancestors = 210963225)

Someone made a mistake somewhere, didn't realize this was supposed to be inverse to above function. Who knows who, but we ought to correct now.

In reply to: 210965477 [](ancestors = 210965477,210963225)

eerhardt · 2018-08-17T16:28:16Z

-    using RawI2 = Int16;
-    using RawI4 = Int32;
-    using RawI8 = Int64;
+    using I1 = SByte;


Would it make sense to just start using the C# keywords inline in the code? That way readers didn't have to map I2 => short in their heads? #Resolved

I think this is generally the case except in this one file, if we consider its nature I feel like this would actually impede readability, at least for me.

So:

AddStd<I2, I1>(Convert); AddStd<I2, I2>(Convert); AddStd<I2, I4>(Convert); AddStd<I2, I8>(Convert); AddStd<I2, R4>(Convert); AddStd<I2, R8>(Convert); AddAux<I2, SB>(Convert);

becomes

AddStd<short, byte>(Convert); AddStd<short, short>(Convert); AddStd<short, int>(Convert); AddStd<short, long>(Convert); AddStd<short, float>(Convert); AddStd<short, double>(Convert); AddAux<short, SB>(Convert);

Maybe. That's hardly disastrous I suppose, and might be easier for some people. Perhaps we ought to revisit though once we've done all the type changes we want to do...

In reply to: 210964331 [](ancestors = 210964331)

TomFinley · 2018-08-17T16:45:43Z

-        private BoolType()
-            : base(typeof(DvBool), DataKind.BL)
+        private readonly string _name;
+


Hmmm. What's this for? Is it still relevant? #Closed

TomFinley · 2018-08-29T18:13:06Z

Change looks OK for now, let's hold off till after 0.5 is cut then we can perhaps do all the changes to the type system in one go. #Resolved

TomFinley

Thanks @codemzs let's hold off till 0.5 though.

TomFinley · 2018-08-29T21:26:30Z

@@ -731,6 +731,12 @@ public void GetMetadata<TValue>(string kind, int col, ref TValue value)
        /// </summary>
        private const ulong ReaderVersion = MissingTextVersion;


ReaderVersion = MissingTextVersion [](start = 28, length = 34)

This was not updated, it should have been. We can read StandardDataTypesVersion so it should be that. #Resolved

TomFinley · 2018-08-29T21:26:43Z

                    Header.VersionToString(header.CompatibleVersion), Header.VersionToString(MetadataVersion));
            }
-            if (header.CompatibleVersion > ReaderVersion)
+            if (header.CompatibleVersion > StandardDataTypesVersion)


CompatibleVersion > StandardDataTypesVersion [](start = 23, length = 44)

Revert these two lines please. #Resolved

TomFinley · 2018-08-29T21:28:55Z

+        /// The first version that removes DvTypes and uses .NET standard
+        /// data types.
+        /// </summary>
+        private const ulong StandardDataTypesVersion = 0x0001000100010006;


StandardDataTypesVersion [](start = 28, length = 24)

Also will need to update the constant in Header.cs. #Resolved

…to types

codemzs · 2018-08-30T19:13:09Z

Sounds good, thanks for reviewing. I was wondering if we should bump the version number for every type conversion PR check-in as this will allow us to check-in PRs without waiting for other PRs to close, thoughts?

In reply to: 417051605 [](ancestors = 417051605)

codemzs · 2018-08-30T19:16:49Z

    /// Missing values are mapped to zero with a true return.

Fix this #Resolved

Refers to: src/Microsoft.ML.Data/Data/Conversion.cs:1199 in 40c01fc. [](commit_id = 40c01fc, deletion_comment = False)

codemzs · 2018-08-30T19:18:46Z

+
+            Assert.True(error);
+
+            //5. Missing value in text to int.


//5. Missing value in text to int. [](start = 12, length = 34)

Empty string #Resolved

codemzs · 2018-08-30T19:18:54Z

+            mapper(ref src, ref dst);
+            Assert.Equal(default, dst);
+
+            //6. Empty string in text to sbyte.


Empty string in text to sbyte. [](start = 14, length = 33)

Missing value #Resolved

…to types # Conflicts: # test/Microsoft.ML.TestFramework/DataPipe/TestDataPipeBase.cs

TomFinley · 2018-08-31T17:05:40Z

I think we can avoid in those cases since I feel like we can easily just have separate codecs probably. In this case we could not avoid I think because we were using the same codec for both purposes.

In reply to: 417434524 [](ancestors = 417434524,417051605)

TomFinley · 2018-08-31T17:08:41Z

datatypes.idv is still shown as being modified in latest commit #Resolved

…to types # Conflicts: # test/BaselineOutput/SingleDebug/SavePipe/TestParquetPrimitiveDataTypes-Data.txt # test/BaselineOutput/SingleDebug/SavePipe/TestParquetPrimitiveDataTypes-Schema.txt # test/BaselineOutput/SingleRelease/SavePipe/TestParquetPrimitiveDataTypes-Data.txt # test/BaselineOutput/SingleRelease/SavePipe/TestParquetPrimitiveDataTypes-Schema.txt # test/data/Parquet/alltypes.parquet

TomFinley · 2018-09-04T23:05:52Z

Hi @codemzs you may want to merge master again, I think our work on APIs and additional tests for pigsty may interfere. #Resolved

…to types

codemzs · 2018-09-07T20:31:45Z

Done.

In reply to: 418545763 [](ancestors = 418545763)

codemzs · 2018-09-20T18:17:37Z

This change was committed via #863

GalOshri and others added 9 commits August 8, 2018 09:54

trigger build.

9ba4dab

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

baf0696

…to types

Datakinds, ColumnTypes and NAHandle.

f7a0fff

Change metadata for categorical indices from DvInt4 to Int32.

1fdfe0d

Conversions class.

734540f

peak, poke, codecs, transforms, evaluators.

10d753c

test.

46299b8

undo DvBool.

a99831b

undo DvBool.

a0ceabb

TomFinley reviewed Aug 16, 2018

View reviewed changes

codemzs changed the title ~~WIP Replace DvInt* with .NET standard data types.~~ Replace DvInt* with .NET standard data types and remove missing value handling for sbyte, short, int and long. Aug 16, 2018

codemzs changed the title ~~Replace DvInt* with .NET standard data types and remove missing value handling for sbyte, short, int and long.~~ Replace DvInt* with .NET standard data types. Aug 16, 2018

codemzs added 2 commits August 16, 2018 14:26

Replace DvInt* with .NET standard types and remove missing value supp…

f30dd7e

…ort for ints.

PR feedback.

a6e5c75

codemzs requested review from Ivanidzo4ka, Zruty0, eerhardt and mandyshieh August 16, 2018 22:08

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

c4bae95

…to types

codemzs commented Aug 16, 2018

View reviewed changes

clean up.

9aea10b

eerhardt reviewed Aug 17, 2018

View reviewed changes

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

db2da0b

…to types

eerhardt reviewed Aug 17, 2018

View reviewed changes

TomFinley reviewed Aug 17, 2018

View reviewed changes

TomFinley approved these changes Aug 29, 2018

View reviewed changes

codemzs changed the title ~~Replace DvInt* with .NET standard data types.~~ Replace DvInt* with .NET standard data types. WIP Aug 29, 2018

TomFinley reviewed Aug 29, 2018

View reviewed changes

codemzs added 3 commits August 29, 2018 16:16

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

9a4f192

…to types

PR feedback.

a7cd3d8

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

40c01fc

…to types

codemzs commented Aug 30, 2018

View reviewed changes

codemzs added 3 commits August 30, 2018 12:25

cleanup.

ad37fb5

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

b9edc92

…to types # Conflicts: # test/Microsoft.ML.TestFramework/DataPipe/TestDataPipeBase.cs

resolve merge conflict.

078cd8c

codemzs added 2 commits September 3, 2018 16:44

PR feedback.

cd7de9f

codemzs changed the title ~~Replace DvInt* with .NET standard data types. WIP~~ Replace DvInt* with .NET standard data types. Sep 4, 2018

codemzs changed the title ~~Replace DvInt* with .NET standard data types.~~ Replace DvInt* with .NET standard data types. WIP Sep 4, 2018

codemzs added 2 commits September 7, 2018 11:19

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

7e80e50

…to types

merge master.

9d4f4d6

codemzs closed this Sep 20, 2018

codemzs deleted the types branch September 20, 2018 18:17

ghost locked as resolved and limited conversation to collaborators Mar 29, 2022


		public void Conv(ref long? src, ref DvInt8 dst) => dst = src ?? DvInt8.NA;
		public void Conv(ref long? src, ref Int64? dst) => dst = src;

		@@ -731,6 +731,12 @@ public void GetMetadata<TValue>(string kind, int col, ref TValue value)
		/// </summary>
		private const ulong ReaderVersion = MissingTextVersion;

Uh oh!

Conversation

codemzs commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codemzs Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Aug 16, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codemzs Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Aug 16, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codemzs Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codemzs Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 17, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 17, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codemzs Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Aug 19, 2018

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 17, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Aug 29, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

TomFinley Aug 29, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

codemzs commented Aug 16, 2018 •

edited

Loading

TomFinley Aug 16, 2018 •

edited

Loading

TomFinley Aug 16, 2018 •

edited

Loading

codemzs Aug 16, 2018 •

edited

Loading

TomFinley Aug 16, 2018 •

edited by codemzs

Loading

codemzs Aug 16, 2018 •

edited

Loading

TomFinley commented Aug 16, 2018 •

edited by codemzs

Loading

TomFinley Aug 16, 2018 •

edited

Loading

codemzs Aug 16, 2018 •

edited

Loading

TomFinley Aug 17, 2018 •

edited

Loading

codemzs Aug 16, 2018 •

edited

Loading

eerhardt Aug 17, 2018 •

edited by codemzs

Loading

eerhardt Aug 17, 2018 •

edited by codemzs

Loading

codemzs Aug 17, 2018 •

edited

Loading

eerhardt Aug 17, 2018 •

edited by codemzs

Loading

TomFinley Aug 17, 2018 •

edited

Loading

TomFinley Aug 17, 2018 •

edited

Loading

TomFinley commented Aug 29, 2018 •

edited by codemzs

Loading

TomFinley Aug 29, 2018 •

edited by codemzs

Loading

TomFinley Aug 29, 2018 •

edited by codemzs

Loading

TomFinley Aug 29, 2018 •

edited by codemzs

Loading

codemzs commented Aug 30, 2018 •

edited

Loading

codemzs Aug 30, 2018 •

edited

Loading

codemzs Aug 30, 2018 •

edited

Loading

TomFinley commented Aug 31, 2018 •

edited by codemzs

Loading

TomFinley commented Sep 4, 2018 •

edited by codemzs

Loading